Skip to content

Conversation

@MatteoPologruto
Copy link
Contributor

@MatteoPologruto MatteoPologruto commented Dec 10, 2025

Motivation

Add gRPC daemon support for the list function.

Change description

Add gRPC daemon support for the list function.

Additional Notes

It can be tested following these steps:

  1. Open a terminal.
  2. Start the Arduino Flasher CLI gRPC daemon:
$ arduino-flasher-cli daemon

Daemon is now listening on 127.0.0.1:50052
{"IP":"127.0.0.1","Port":"50052"}
  1. Open another terminal.
  2. Run the following grpcurl command in the new terminal to create a gRPC client instance:
$ grpcurl \
  -plaintext \
  -import-path ./rpc \
  -proto cc/arduino/flasher/v1/commands.proto \
  127.0.0.1:50052 \
  cc.arduino.flasher.v1.Flasher.List

Reviewer checklist

  • PR addresses a single concern.
  • PR title and description are properly filled.
  • Changes will be merged in main.
  • Changes are covered by tests.
  • Logging is meaningful in case of troubleshooting.

@MatteoPologruto MatteoPologruto self-assigned this Dec 10, 2025
@MatteoPologruto MatteoPologruto added the enhancement New feature or request label Dec 10, 2025
@MatteoPologruto MatteoPologruto linked an issue Dec 10, 2025 that may be closed by this pull request
@MatteoPologruto MatteoPologruto marked this pull request as ready for review December 10, 2025 08:30
@MatteoPologruto MatteoPologruto requested review from a team and cmaglie December 10, 2025 08:30
Comment on lines 35 to 38
latest := false
if manifest.Releases[i].Version == manifest.Latest.Version {
latest = true
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
latest := false
if manifest.Releases[i].Version == manifest.Latest.Version {
latest = true
}
latest := (manifest.Releases[i].Version == manifest.Latest.Version)

Also, it's unclear why you're looping in reverse here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The json manifest we get contains the latest release in the last element of the array, but we decided that we want to return it as the first item of Releases

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not depend on how the input data is sorted; it happens to be that way, but it is not guaranteed.

Base automatically changed from flash-command-refactoring to main December 10, 2025 15:15
protoc:compile:
desc: Compile protobuf definitions
cmds:
- buf dep update
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would install buf as a go tool. You can run go get --tool github.com/bufbuild/buf to add the latest buf version in the go.mod, and then here you can write

Suggested change
- buf dep update
- go tool buf dep update


daemonCommand.Flags().StringVar(&daemonPort,
"port", "50052",
i18n.Tr("The TCP port the daemon will listen to"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
i18n.Tr("The TCP port the daemon will listen to"))
i18n.Tr("The TCP port the daemon will listen to, 0 for random port"))

Comment on lines +30 to +33
"github.com/arduino/arduino-flasher-cli/cmd/feedback"
"github.com/arduino/arduino-flasher-cli/cmd/i18n"

flasher "github.com/arduino/arduino-flasher-cli/rpc/cc/arduino/flasher/v1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"github.com/arduino/arduino-flasher-cli/cmd/feedback"
"github.com/arduino/arduino-flasher-cli/cmd/i18n"
flasher "github.com/arduino/arduino-flasher-cli/rpc/cc/arduino/flasher/v1"
"github.com/arduino/arduino-flasher-cli/cmd/feedback"
"github.com/arduino/arduino-flasher-cli/cmd/i18n"
flasher "github.com/arduino/arduino-flasher-cli/rpc/cc/arduino/flasher/v1"

}

daemonCommand.Flags().StringVar(&daemonPort,
"port", "50052",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a random port by default

Suggested change
"port", "50052",
"port", "0",

Comment on lines +35 to +39
latest := false
if manifest.Releases[i].Version == manifest.Latest.Version {
latest = true
}
resp.Releases = append(resp.Releases, &flasher.Release{BuildId: manifest.Releases[i].Version, Latest: latest})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot this be simplified in the following?

Suggested change
latest := false
if manifest.Releases[i].Version == manifest.Latest.Version {
latest = true
}
resp.Releases = append(resp.Releases, &flasher.Release{BuildId: manifest.Releases[i].Version, Latest: latest})
resp.Releases = append(resp.Releases, &flasher.Release{
BuildId: manifest.Releases[i].Version,
Latest: manifest.Releases[i].Version == manifest.Latest.Version
})

list.NewListCmd(),
download.NewDownloadCmd(),
version.NewVersionCmd(Version),
daemon.NewDaemonCommand(srv),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
daemon.NewDaemonCommand(srv),
daemon.NewDaemonCommand(service.NewFlasherServer()),

Comment on lines +42 to +43
srv := service.NewFlasherServer()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
srv := service.NewFlasherServer()


func (r daemonResult) String() string {
j, _ := json.Marshal(r)
return fmt.Sprintln(i18n.Tr("Daemon is now listening on %s:%s", r.IP, r.Port)) + fmt.Sprintln(string(j))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you including the JSON?

Suggested change
return fmt.Sprintln(i18n.Tr("Daemon is now listening on %s:%s", r.IP, r.Port)) + fmt.Sprintln(string(j))
return fmt.Sprintln(i18n.Tr("Daemon is now listening on %s:%s", r.IP, r.Port))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[App Lab integration] List the available images

3 participants